Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: _JSXStyle is not defined #716

Closed

Conversation

morrisallison
Copy link

Description

addDefault only accepts a nameHint and doesn't guarantee the name of the resulting default import. The identifier returned by addDefault should be referenced.

This change renames the default import hint to _JSXStyleImport, and adds a variable declaration after the import to assign the default import to _JSXStyle.

As a result, instead of this being output in some cases:

import _JSXStyle2 from 'styled-jsx/style';

This will be output:

import _JSXStyleImport2 from 'styled-jsx/style';
var _JSXStyle = _JSXStyleImport2;

Admittedly, a better solution would be to use a named export like in #690, but this is quick fix that should unblock those who are having issues with later releases.

Related

Fixes #695
Fixes #696
Fixes #711

`addDefault` only accepts a `nameHint` and doesn't guarantee
the name of the resulting default import. The identifier returned
by `addDefault` should be referenced.

This change renames the default import hint to `_JSXStyleImport`,
and adds a variable declaration after the import to
assign the default import to `_JSXStyle`.

As a result, instead of this being output in some cases:

```js
import _JSXStyle2 from 'styled-jsx/style';
```

This will be output:

```js
import _JSXStyleImport2 from 'styled-jsx/style';
var _JSXStyle = _JSXStyleImport2;
```
@morrisallison morrisallison requested a review from giuseppeg as a code owner May 21, 2021 00:00
Comment on lines +83 to +87
var _JSXStyle = _style.default;␊
function Test() {␊
return <div>␊
<_style.default id={_App.default.__hash}>{_App.default}</_style.default>␊
<_JSXStyle id={_App.default.__hash}>{_App.default}</_JSXStyle>␊
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This pretty much explains what's going on.

@morrisallison morrisallison changed the title Fix _JSXStyle is not defined fix: _JSXStyle is not defined May 21, 2021
Using `node.body.splice` broke tests for external CSS.
@giuseppeg
Copy link
Collaborator

giuseppeg commented May 24, 2021

Hi @morrisallison thank you for taking the time to look into this.

It seems that the problem right now is that _JSXStyle might already exist in the module and addDefault generates an identifier with a different name. Adding a mapping to _JSXStyleImport is somehow similar to renaming _JSXStyle and hoping that it will be unique.

When @jaydenseric introduced this change I anticipated this issue: #684 (comment)

If we want to fix this, we will have to generate the identifier (the name of the import specifier) upfront when we enter the Program and pass it to the helpers that add the <_JSXStyle> elements like I said in the comment.

Please let me know if I misunderstood the issue that you are trying to fix.

@morrisallison
Copy link
Author

Hi @morrisallison thank you for taking the time to look into this.

It seems that the problem right now is that _JSXStyle might already exist in the module and addDefault generates an identifier with a different name. Adding a mapping to _JSXStyleImport is somehow similar to renaming _JSXStyle and hoping that it will be unique.

When @jaydenseric introduced this change I anticipated this issue: #684 (comment)

If we want to fix this, we will have to generate the identifier (the name of the import specifier) upfront when we enter the Program and pass it to the helpers that add the <_JSXStyle> elements like I said in the comment.

Please let me know if I misunderstood the issue that you are trying to fix.

Yup, that's accurate 👍🏽 However, I didn't want to do a lot of refactoring to reference the identifier everywhere. This change is intended to be quick (maybe dirty) fix to unblock. A proper fix will be more involved like you described.

@giuseppeg
Copy link
Collaborator

I'd rather fix this properly. We need to create the import identifier when we enter the Program, put it in state and use its name property (id.node.name) instead of STYLE_COMPONENT:

https://github.com/vercel/styled-jsx/blob/master/src/_utils.js#L383
https://github.com/vercel/styled-jsx/blob/master/src/_utils.js#L261
https://github.com/vercel/styled-jsx/blob/master/src/_utils.js#L624

@morrisallison
Copy link
Author

Closing, because I don't have the bandwidth to work on further changes, but I'm glad this could help identify the problem and solution.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants